Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[new opmath 4] Update tests to pass under new opmath #5335

Merged
merged 225 commits into from
Mar 25, 2024

Conversation

Qottmann
Copy link
Contributor

@Qottmann Qottmann commented Mar 7, 2024

Update tests to pass under new opmath using a context manager and fixture

Branching: #5269 > #5216 > #5322 > #5335

  • Update Hamiltonian tests
  • remove all occurences of enable/disable_new_opmath (currently still missing: in qchem and group_observables)
  • tests/ops/op_math pass *see caveat below in test_exp.py
  • tests/ops/qubit/ pass (includes hamiltonian tests)
  • tests/ops/ pass *except some blockers in tests/ops/functions/test_assert_valid.py
  • tests/pulse pass *see pickle blocker, could be local
  • tests/tape/
  • tests/tape/
  • tests/resource/
  • tests/qnn/
  • tests/qinfo/
  • tests/numpy/
  • tests/logging/
  • tests/kernels/
  • tests/fermi/
  • tests/fourier/
  • tests/math/

Note that there are currently some tests and parts of the codebase that enable/disable new opmath, which affects other tests by changing the global variable __use_new_opmath

Blockers:

  • test_exp.py::TestDecomposition::test_generator_decomposition
  • tests/ops/functions/test_generator.py
  • tests/ops/functions/test_is_commuting.py require changes in qml.is_commuting
  • tests/ops/functions/test_assert_valid.py::test_generated_list_of_ops
  • tests/ops/functions/test_assert_valid.py::test_explicit_list_of_ops
  • qml.equal handles LinearCombination correctly and tests in tests/ops/functions/test_equal.py::TestObservablesComparisons pass also for LinearCombination
  • >> Update tests/ops/functions/test_equal.py
  • tests/pulse/test_pytree.py::test_standard_validity E AttributeError: Can't pickle local object 'test_standard_validity.<locals>.f1'

[sc-57841]

@Qottmann
Copy link
Contributor Author

Qottmann commented Mar 7, 2024

On this branch / PR the Hamiltonian tests pass now. We should now be able to see what breaks and define and distribute tasks to update tests based on that.

Note however that there are some false positives as some tests actively enable and disable new opmath, which under the new default just globally disable new opmath in the CI runner of that test afterwards. So to get a clearer picture we need to remove all those first.

Theoretically, there should be no need to update the codebase, solely the tests. However there may be some unknowns.

@Qottmann Qottmann changed the title Update tests to pass under new opmath [new opmath 4] Update tests to pass under new opmath Mar 8, 2024
Qottmann and others added 16 commits March 20, 2024 17:40
**Context:**
Updates the qchem generators to return as either a `qml.ops.Hamiltonian`
or `qml.ops.LinearCombination` (depending on whether op_math is enabled)
as part of standardizing generators and keeping them consistent with the
`op_math` setting.

**Description of the Change:**
These ones already returned a new opmath operator (SProd) if
`active_new_opmath` and a legacy operator (Hamiltonian) otherwise,
because they are created using dunder methods rather than explicit
constructors. This change makes it so they will return
`LinearCombination` instead, because this is more analogous to
`Hamiltonian`, and some of the generator behaviour depends on that.

Added tests for both old and new opmath checking the generator return
type, and tagged the parameter frequency tests (functionality dependent
on `op.generator()`) to run with both.

**Benefits:**
- The qchem op generators are consistently either "`qml.Hamiltonian`"
(whatever that may mean in the current context) or `SparseHamiltonian`.
- We can remove one of our `xfail` marks in the optimize folder that was
a result of the discrepancy here
- We can revert a test change that we missed previously that represented
a real change in functionality and *should* have been an `xfail`

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: qottmann <korbinian.kottmann@gmail.com>
Co-authored-by: Alex Preciado <alex.preciado@xanadu.ai>
Co-authored-by: Pietropaolo Frisoni <pietropaolo.frisoni@xanadu.ai>
**Context:**
`TestHamiltonianSamples` fails in ham-tests

**Description of the Change:**
Update `qml.Hamiltonian` to `qml.ops.Hamiltonian` in `isinstance` checks
such that it always refer to the old `Hamiltonian`

[sc-59163]
After looking at the tests for a bit and realizing what was going on, I
would recommend these generators return new_opmath operators regardless
of whether new_opmath is installed, because the new_opmath was
explicitly used to add functionality that isn't available with
`qml.Hamiltonian`:

- the "Identity with no wires" functionality only works with new opmath,
and is needed for `ControlledGlobalPhase`. Changing `Identity` and
`Controlled` is just undoing #5194.
- a Hamiltonian is not suitable for the qutrit operators, because it
can't return a matrix for a qutrit system, breaking parameter
frequencies

I've added a comment on each of the relevant generator functions, since
this will differ from the standard format, and that's all.

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: qottmann <korbinian.kottmann@gmail.com>
Co-authored-by: Alex Preciado <alex.preciado@xanadu.ai>
Co-authored-by: Pietropaolo Frisoni <pietropaolo.frisoni@xanadu.ai>
**Context:** After enabling the new operator arithmetic by default, we
want the generators in the source code to return a `LinearCombination`
instance or a `Hamiltonian` instance wherever possible.

**Description of the Change:** The generators touched in this PR are
modified so that they return `qml.Hamiltonian` instead of `Sum`, `Prod`,
or `Sprod` instances. When opmath is enabled, `qml.Hamiltonian` points
to `pennylane.ops.op_math.linear_combination.LinearCombination`, and
when it is disabled, it points to
`pennylane.ops.qubit.hamiltonian.Hamiltonian`. This ensures that the
appropriate instance is used consistently.
Note that the generators unchanged in this PR are modified (wherever
possible) in #5410 , #5411 , #5412 (including the changelog entry).

**Benefits:** A more coherent choice depending on whether opmath is
enabled or disabled.

**Possible Drawbacks:** None that I can think of, except that old opmath
would be deprecated in the future. Therefore, some precautions that have
been taken here (to ensure that tests associated with generators work
even with opmath disabled) might become useless.

**Related GitHub Issues:** None.

[sc-57982]

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: lillian542 <Lillian.frederiksen@xanadu.ai>
Co-authored-by: qottmann <korbinian.kottmann@gmail.com>
Co-authored-by: Alex Preciado <alex.preciado@xanadu.ai>
…ithmetic (#5421)

**Context:**
We are generally treating "Hamiltonian" as meaning alternately
`qml.ops.Hamiltonian` or `qml.ops.LinearCombination` depending on
whether or not `new_opmath` is enabled.

The current implementation of `qml.generator(op, format="hamiltonian")`
mostly respects that, but uses `convert_to_legacy_H` if the generator is
a `Sum`, `Prod` or `SProd` even with `new_opmath` is enabled, breaking
the convention.

**Description of the Change:**
We add `convert_to_H`, which converts any arithmetic operator to a
conceptual Hamiltonian - either `qml.ops.Hamiltonian` or
`qml.ops.LinearCombination`. We use the new `convert_to_H` function in
the `qml.generator` method. The `convert_to_legacy_H` function continues
to convert specifically to a legacy Hamiltonian regardless of the global
setting for `new_opmath`.

**Benefits:**
Consistency, no one will ask for a generator with `new_opmath` enabled
and get a legacy Hamiltonian.

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: qottmann <korbinian.kottmann@gmail.com>
Co-authored-by: Alex Preciado <alex.preciado@xanadu.ai>
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Qottmann and others added 5 commits March 25, 2024 15:18
`hamiltonian.py` defines the legacy `Hamiltonian`. It should not be
returning `LinearCombination` in its own methods even with new op math
enabled.

Co-authored-by: qottmann <korbinian.kottmann@gmail.com>
#5426)

[sc-59249]

---------

Co-authored-by: qottmann <korbinian.kottmann@gmail.com>
@Qottmann Qottmann merged commit 3d4ec50 into linearcombination Mar 25, 2024
37 checks passed
@Qottmann Qottmann deleted the ham-tests branch March 25, 2024 15:18
Qottmann added a commit that referenced this pull request Mar 25, 2024
…ian` (#5216)

Branching: #5269 >
#5216 >
#5322 >
#5335

The basic idea is to have a clone of `qml.Hamiltonian` with a better
name, `LinearCombination` - that inherits from `CompositeOp` and plays
nice with new opmath. The motivation is that sometimes it is still handy
to have an operator class for which you _know_ there is no funny nesting
and that has `coeffs` and `ops` separately on demand without processing.

ToDo

- [x] basic init
- [x] `tests/ops/op_math/test_linear_combination.py` pass
- [x] update map_wires
- [x] all tests pass
- [x] add pauli_rep
- [x] add operands attribute and make iterable (?)
- [x] update string repr
- [x] upgrade to internally use new opmath
- [x] update tests with default new opmath
- [x] dunder math methods support with new opmath
- [x] simplify and speed-up `simplify()`, also make sure to not act
in-place
- [x] Could we make the matrix generation workflow more general like
Sum?
- [x] Also, deferring to the pauli rep method for matrix generation
leads to substantial performance improvements.
- [x] toggle `__use_new_opmath`
- [x] tests_linear_combination pass
- [x] Integration tests
- [x] get all linear combination tests to pass locally
- [x] utilize super().sparse_matrix
- [x] use grouping functionality from Sum that Mudit is adding in
#5179
- [x] update matmul to handle other LinearCombination instances
- [x] remove top level import
- [x] remove unnecessary copies
- [x] add diagonalizing gates
- [x] bugfix trivial case of simplify empty LinearCombination
- [x] add trivial case for is_hermitian
- [ ] torch differentiation with simplify=True in constructor
- [x] changelog

Optional features (likely to be included in a follow-up)
- [x] `qml.Hamiltonian` points to either old Hamiltonian or
LinearCombination depending on `__use_new_opmath`
- [x] support Hermitian
- [ ] matrix method (optional)
- [ ] adjoint method (optional)
- [ ] batching support (optional)
- [ ] qml.dot points to LinearCombination
- [ ] take care of xfails (mostly about raiseing errors in other parts
of the codebase)
- [ ] update logic of adjoint differentiation to catch attempt to
differentiate lincomb coeffs
- [x] support in qml.equal
- [x]  add special matmul with other product or non-composite ops
- [x] add special matmul with other LinearCombination


[sc-56704]

---------

Co-authored-by: lillian542 <38584660+lillian542@users.noreply.github.com>
Co-authored-by: Astral Cai <astral.cai@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Pietropaolo Frisoni <pietropaolo.frisoni@xanadu.ai>
Co-authored-by: albi3ro <chrissie.c.l@gmail.com>
Co-authored-by: Utkarsh <utkarshazad98@gmail.com>
Co-authored-by: lillian542 <Lillian.frederiksen@xanadu.ai>
Co-authored-by: Alex Preciado <alex.preciado@xanadu.ai>
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Vincent Michaud-Rioux <vincentm@nanoacademic.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
Co-authored-by: Mikhail Andrenkov <mikhail@xanadu.ai>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Qottmann added a commit that referenced this pull request Mar 25, 2024
PR to toggle the `__use_new_opmath` and make it default.
This will require several changes in the codebase (and further updates
to demos, dataset and plugins).
Bigger changes should be offloaded in separate PRs. Small updates can be
gathered here.

Branching: #5269 >
#5216 >
#5322 >
#5335

- [ ] Make sure lightning works

```python
import os, sys
os.environ["OMP_NUM_THREADS"]="8"
import pennylane as qml
from pennylane import numpy as np
from timeit import default_timer as timer

if __name__ == "__main__":
    rng = np.random.default_rng(seed=1337)  # make the results reproducable
    mol = qml.data.load("qchem", molname="H2O", bondlength=0.958, basis="STO-3G")[0]

    hf_state = mol.hf_state
    ham = mol.hamiltonian
    wires = ham.wires
    dev = qml.device("lightning.qubit", wires=wires, batch_obs=True)

    n_electrons = mol.molecule.n_electrons
    singles, doubles = qml.qchem.excitations(n_electrons, len(wires))
    hf_state.requires_grad = False
    hf_ops = []

    for idx,i in enumerate(hf_state):
        if i==1:
            hf_ops.append((qml.PauliX, idx))

    @qml.qnode(dev, diff_method="adjoint")
    def cost(weights):
        for (x,i) in hf_ops: # HF state
            x(i)
        for idx,s in enumerate(singles):
            qml.SingleExcitation(weights[idx], wires=s)
        for idx,d in enumerate(doubles):
            qml.DoubleExcitation(weights[idx+len(singles)], wires=d)
        return qml.expval(ham)

    params = qml.numpy.array(rng.normal(0, np.pi, len(singles) + len(doubles)))
    opt = qml.GradientDescentOptimizer(stepsize=0.5)

    # store the values of the circuit parameter
    angle = [params]
    max_iterations = 50
    procs = int(os.getenv("PL_FWD_BATCH", "0"))
    pre_s = f"qubits={len(wires)},num_terms={len(ham.terms()[0])},procs={procs},"

    energies = []

    for n in range(max_iterations):
        start_grad = timer()
        params, prev_energy = opt.step_and_cost(cost, params)
        energies.append(prev_energy)
        end_grad = timer()
        angle.append(params)
        print(f"{pre_s},Step={n},Time_grad={end_grad-start_grad}")

    start_fwd = timer()
    energy = cost(params)
    energies.append(energy)
    end_fwd = timer()
    print(f"Energies={energies},Time_fwd={end_fwd - start_fwd}")
```

---------

Co-authored-by: lillian542 <38584660+lillian542@users.noreply.github.com>
Co-authored-by: Astral Cai <astral.cai@xanadu.ai>
Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
Co-authored-by: Christina Lee <christina@xanadu.ai>
Co-authored-by: Pietropaolo Frisoni <pietropaolo.frisoni@xanadu.ai>
Co-authored-by: albi3ro <chrissie.c.l@gmail.com>
Co-authored-by: Utkarsh <utkarshazad98@gmail.com>
Co-authored-by: lillian542 <Lillian.frederiksen@xanadu.ai>
Co-authored-by: Alex Preciado <alex.preciado@xanadu.ai>
Co-authored-by: Thomas R. Bromley <49409390+trbromley@users.noreply.github.com>
Co-authored-by: Vincent Michaud-Rioux <vincentm@nanoacademic.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Nathan Killoran <co9olguy@users.noreply.github.com>
Co-authored-by: Matthew Silverman <matthews@xanadu.ai>
Co-authored-by: Mikhail Andrenkov <mikhail@xanadu.ai>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants